Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Set output.experimentalMinChunkSize to 0, to counter a change in [email protected] #1449

Merged
merged 3 commits into from
May 26, 2023
Merged

Conversation

ijlee2
Copy link
Contributor

@ijlee2 ijlee2 commented May 25, 2023

Background

A few independent incidences

After updating rollup to 3.22.0, the package embroider-css-modules worked, in the sense that the consuming app (a test app) continued to run and all tests for the test app continued to pass. I noticed, however, that the lint:types script of the test app failed because embroider-css-modules didn't create dist/index.d.ts in the same way as before.

Error messages
app/components/widgets/widget-4/memo/actions.gts:3:28 - error TS2306: File '/packages/embroider-css-modules/dist/index.d.ts' is not a module.

3 import { localClass } from 'embroider-css-modules';
                             ~~~~~~~~~~~~~~~~~~~~~~~

app/components/ui/form/field.gts:4:28 - error TS2306: File '/packages/embroider-css-modules/dist/index.d.ts' is not a module.

4 import { localClass } from 'embroider-css-modules';
What dist looked like before updating rollup
What dist looked like after updating rollup

After updating rollup to 3.22.0 in another package (in another repo), I noticed that a different file called dist/template-registry.d.ts had been created differently. The test app for that package also failed lint:types as a result.

What dist/template-registry.d.ts looked like before updating rollup
import UiSpinnerComponent from "./components/ui/spinner.js";
interface UiSpinnerRegistry {
    'Ui::Spinner': typeof UiSpinnerComponent;
}
export { UiSpinnerRegistry as default };
What dist/template-registry.d.ts looked like after updating rollup
export {};

Finally, on Discord, @jeffsawatzky described encountering a similar issue.

And it [Glint] is mostly working, except that the registry that I define in template-registry.ts don't get saved in template-registry.d.ts after build. Instead that file is blank, and the code I put in template-registry.ts ends up in index.d.ts. I've looked at other examples, but I haven't been able to figure this out.

Possible root cause

I reported the issue to the maintainers of rollup in rollup/rollup#5008.

@lukastaegert noticed, the problem seemed to be limited to *.d.ts files, e.g. dist/index.d.ts, dist/template-registry.d.ts. If so, rollup-plugin-ts (which we use to support TypeScript) or one of its dependencies, might have made an assumption about chunks that is not valid as of [email protected].

Proposed solution

Lukas (who maintains rollup) and I both think, a good option is for @embroider/addon-dev to explicitly set output.experimentalMinChunkSize to 0.

Maybe explicitly setting output.experimentalMinChunkSize:0 in the shared config is not a bad idea for now. The reasoning for setting it to 1 was that there should not be adverse effects to users, but it should avoid some empty chunks. It would still be helpful if someone with more knowledge about the workings of rollup-plugin-dts could look into this and determine if the empty files are a bug in the plugin or in Rollup. In the worst case, we could change the default back to 0 until the next major version.

This way, an addon maintainer doesn't need to set the value themselves in rollup.config.js:

/* rollup.config.{js,mjs} */

import { Addon } from '@embroider/addon-dev/rollup';
import typescript from 'rollup-plugin-ts';

const addon = new Addon({
  srcDir: 'src',
  destDir: 'dist',
});

export default {
  output: {
    ...addon.output(),
    experimentalMinChunkSize: 0,  // <-- Current workaround for addon developers
  },

  plugins: [ ... ],
};

At the same time, the maintainers of rollup could continue to iterate on the default value of experimentalMinChunkSize for v3 and v4.

Related links

@ijlee2 ijlee2 changed the title Set output.experimentalMinChunkSize to 0, to counter a change in [email protected] [bugfix] Set output.experimentalMinChunkSize to 0, to counter a change in [email protected] May 25, 2023
@ijlee2 ijlee2 marked this pull request as ready for review May 25, 2023 06:43
Comment on lines +78 to +79
experimentalMinChunkSize: 0,
format: 'es',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I alphabetized the keys. When there is a list of configuration keys, people tend to add a new one to the end of the list. I find unsorted lists not easy to understand and maintain.

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice write up!

@NullVoxPopuli
Copy link
Collaborator

For testing, can you bump the rollup version in tests/scenarios to 3.22+?

@ijlee2
Copy link
Contributor Author

ijlee2 commented May 25, 2023

For testing, can you bump the rollup version in tests/scenarios to 3.22+?

Yep, I can try.

@@ -42,7 +42,7 @@
"@types/fs-extra": "^9.0.12",
"@types/minimatch": "^3.0.4",
"@types/yargs": "^17.0.3",
"rollup": "^2.58.0",
"rollup": "^3.23.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/rollup/rollup/releases/tag/v3.0.0 for breaking changes that might affect @embroider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli Can you run CI for me?

@ijlee2
Copy link
Contributor Author

ijlee2 commented May 26, 2023

Yesterday, 2 out of 200 checks (both on windows env, if I recall) failed due to a test timeout. I rebased the branch to rerun the CI and 1 check failed because of a timeout.

Can you manually retry the failing check?

@NullVoxPopuli
Copy link
Collaborator

Thank you!

@NullVoxPopuli NullVoxPopuli merged commit 77dd32c into embroider-build:main May 26, 2023
@ijlee2 ijlee2 deleted the bugfix-for-rollup-3.22.0 branch May 26, 2023 11:58
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants